docs: clarify the new default client cipher list#16474
Merged
htuch merged 2 commits intoenvoyproxy:mainfrom May 23, 2021
Merged
docs: clarify the new default client cipher list#16474htuch merged 2 commits intoenvoyproxy:mainfrom
htuch merged 2 commits intoenvoyproxy:mainfrom
Conversation
|
Hi @desimone, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
PiotrSikora
previously approved these changes
May 14, 2021
Contributor
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
You need to fix format (run ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format', ./ci/do_ci.sh fix_format or ./tools/proto_format/proto_format.sh fix), otherwise LGTM. Thanks!
Contributor
|
cc @htuch These changes need to be applied to https://github.com/envoyproxy/envoy/blob/d26b16c328e594e80ea7fba6243e336d2d2be6aa/api/envoy/extensions/transport_sockets/tls/v3/common.proto And the scripts that Piotr mentions in #16474 (review) need to be run to regenerate shadows from the canonical source mentioned above. |
Contributor
Author
|
Reapplied the changes to the correct proto. \cc @travisgroth can you help me with an assist on those scripts? They don't work with my M1 processor :/ |
htuch
reviewed
May 16, 2021
Member
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Thanks, defer to @PiotrSikora to sign-off on the correctness of the cipher lists.
| // ECDHE-ECDSA-AES128-GCM-SHA256 | ||
| // ECDHE-RSA-AES128-GCM-SHA256 | ||
| // ECDHE-ECDSA-AES256-GCM-SHA384 | ||
| // ECDHE-RSA-AES256-GCM-SHA384 |
Member
There was a problem hiding this comment.
It would be nice if we could generate these from some canonical location that is shared with code. If we aren't going to change frequently this is fine though.
Contributor
I already did (#16474 (review)). |
htuch
suggested changes
May 18, 2021
Member
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo nit. Please fix format as suggested to @PiotrSikora
|
@PiotrSikora I'm getting an error running the format commands: Is there any setup needed to run that script? I tried via docker, which also failed, but it gave me no error output to triage. |
Contributor
It looks that you're missing It's weird that the Docker would fail, since it should be self-contained, and it works fine for me: |
Member
|
Maybe @travisgroth isn't running in Docker.. |
% ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format'
No remote cache is set, skipping setup remote cache.
ENVOY_SRCDIR=/source
ENVOY_BUILD_TARGET=//source/exe:envoy-static
ENVOY_BUILD_ARCH=x86_64
$TEST_TMPDIR defined: output root default is '/build/tmp' and max_idle_secs default is '15'.
Starting local Bazel server and connecting to it...
Skip setting up Envoy Filter Example.
building using 4 CPUs
building for x86_64
clang toolchain with libc++ configured
fix_format...
api/envoy/extensions/transport_sockets/tls/v3/common.proto
protoxform_test...
$TEST_TMPDIR defined: output root default is '/build/tmp' and max_idle_secs default is '15'.
Starting local Bazel server and connecting to it...
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 2 packages loaded
Loading: 2 packages loaded
$TEST_TMPDIR defined: output root default is '/build/tmp' and max_idle_secs default is '15'.
INFO: Analyzed target //tools/testdata/protoxform:fix_protos (65 packages loaded, 1241 targets configured).
INFO: Found 1 target...
INFO: From CcCmakeMakeRule external/envoy/bazel/foreign_cc/zlib/include [for host]:
ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/envoy/bazel/foreign_cc/BUILD:349:21: TreeArtifact external/envoy/bazel/foreign_cc/zlib/include was not created
ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/envoy/bazel/foreign_cc/BUILD:349:21: TreeArtifact external/envoy/bazel/foreign_cc/copy_zlib/zlib was not created
ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/envoy/bazel/foreign_cc/BUILD:349:21: not all outputs were created or valid
Aspect //tools/protoxform:protoxform.bzl%protoxform_aspect of //tools/testdata/protoxform:fix_protos failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 13.459s, Critical Path: 4.92s
INFO: 27 processes: 20 internal, 7 processwrapper-sandbox.
FAILED: Build did NOT complete successfully |
|
@PiotrSikora thanks that did the trick. |
Signed-off-by: Bobby DeSimone <bobbydesimone@gmail.com>
Contributor
|
( |
Contributor
Author
|
LMK if you need anything additional on my end |
Member
|
Try mergre main and push again. Thanks. |
Contributor
Author
|
|
Member
|
/lgtm api |
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
These changes clarify that as of v1.16 the default cipher suite is different for client and servers. Risk Level: Low Testing: N/A Docs Changes: Yes Release Notes: N/A Platform Specific Features: N/A Fixes envoyproxy#16469 Signed-off-by: Bobby DeSimone <bobbydesimone@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: docs: clarify the new default client cipher list
Additional Description: These changes clarify that as of v1.16 the default cipher suite is different for client and servers.
Risk Level: Low
Testing: N/A
Docs Changes: Yes
Release Notes: N/A
Platform Specific Features: N/A
Fixes #16469